daras_ai_v2/gooey_builder.py (2)

3-3: Defer heavy Django model import to reduce server import time

Importing BotIntegration at module import time can slow startup. Lazily import inside the functions instead.

Apply this diff:

-from bots.models import BotIntegration
+# Defer heavy Django model import to function scope to reduce import time

And inside both functions:

 def render_gooey_builder_inline(page_slug: str, builder_state: dict):
+    from bots.models import BotIntegration
     if not settings.GOOEY_BUILDER_INTEGRATION_ID:
         return
@@
 def render_gooey_builder(page_slug: str, builder_state: dict):
+    from bots.models import BotIntegration
     if not settings.GOOEY_BUILDER_INTEGRATION_ID:
         return

Also applies to: 11-15, 66-74

---

12-14: Avoid hard-coding hostname; derive at runtime or accept a parameter

Using hostname="gooey.ai" may break staging/custom hosts and Google Maps API hostname checks. Consider deriving from settings.APP_BASE_URL or passing hostname from the caller.

Example:

from furl import furl
hostname = furl(settings.APP_BASE_URL).host or "gooey.ai"
config = bi.get_web_widget_config(hostname=hostname, target="#gooey-builder-embed")

Also applies to: 16-24, 30-33

daras_ai_v2/base.py (1)

417-446: Unify default sidebar open state with routers.root to avoid first-load flicker

render_sidebar uses use_sidebar("builder-sidebar") with default default_open=True, but routers.root initializes the same key with default_open=False. Consider aligning defaults in both places (False) to prevent state desync on first load.

Would you like a small patch to centralize this default in a constant?

widgets/sidebar.py (3)

41-47: Use dict.pop instead of membership test + del (RUF051)

Simplify and avoid KeyError race.

Apply this diff:

-    if current_time - last_load_time > 0.5:
-        # Fresh page load - clear mobile state
-        mobile_key = key + ":mobile"
-        if mobile_key in session:
-            del session[mobile_key]
+    if current_time - last_load_time > 0.5:
+        # Fresh page load - clear mobile state
+        mobile_key = key + ":mobile"
+        session.pop(mobile_key, None)

---

127-134: Remove unused enumerate index (B007)

The loop counter i is unused.

Apply this diff:

-def sidebar_item_list(is_sidebar_open, current_url=None):
-    for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS):
+def sidebar_item_list(is_sidebar_open, current_url=None):
+    for url, label, icon in settings.SIDEBAR_LINKS:

---

41-43: Fix misleading comment about time threshold

Comment says “1 second” but code uses 0.5s.

Update to: “If more than 0.5 seconds has passed since last load…”

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/base.py (1)

1252-1274: Do not pass entire session_state to the widget; use the filtered approach in the commented code.

This change still passes gui.session_state wholesale to the client widget (line 1254), which can expose unrelated or sensitive session data. The previous review flagged this exact issue, and the commented code (lines 1255-1274) demonstrates the safer, filtered approach.

Apply this diff to restore the filtered payload:

-        render_gooey_builder_inline(self.slug_versions[-1], gui.session_state)
-        # render_gooey_builder(
-        #     page_slug=self.slug_versions[-1],
-        #     builder_state=dict(
-        #         status=dict(
-        #             error_msg=gui.session_state.get(StateKeys.error_msg),
-        #             run_status=gui.session_state.get(StateKeys.run_status),
-        #             run_time=gui.session_state.get(StateKeys.run_time),
-        #         ),
-        #         request=extract_model_fields(
-        #             model=self.RequestModel, state=gui.session_state
-        #         ),
-        #         response=extract_model_fields(
-        #             model=self.ResponseModel, state=gui.session_state
-        #         ),
-        #         metadata=dict(
-        #             title=self.current_pr.title,
-        #             description=self.current_pr.notes,
-        #         ),
-        #     ),
-        # )
+        builder_state = dict(
+            status=dict(
+                error_msg=gui.session_state.get(StateKeys.error_msg),
+                run_status=gui.session_state.get(StateKeys.run_status),
+                run_time=gui.session_state.get(StateKeys.run_time),
+            ),
+            request=extract_model_fields(
+                model=self.RequestModel, state=gui.session_state
+            ),
+            response=extract_model_fields(
+                model=self.ResponseModel, state=gui.session_state
+            ),
+            metadata=dict(
+                title=self.current_pr.title,
+                description=self.current_pr.notes,
+            ),
+        )
+        render_gooey_builder_inline(self.slug_versions[-1], builder_state)

daras_ai_v2/gooey_builder.py (1)

7-24: Consider documenting the expected structure and security implications of builder_state.

The builder_state parameter accepts any dict and passes it to the client widget via variables. Adding a docstring or type hint clarifying the expected structure (e.g., status/request/response/metadata) and noting that sensitive data should be filtered out would help prevent misuse.

Example:

 def render_gooey_builder_inline(page_slug: str, builder_state: dict):
+    """
+    Render the Gooey builder widget in inline mode.
+    
+    Args:
+        page_slug: The workflow page slug
+        builder_state: Filtered state dict containing only non-sensitive data
+            (e.g., status, request, response, metadata). Do not pass raw session_state.
+    """
     if not settings.GOOEY_BUILDER_INTEGRATION_ID:
         return

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/base.py (1)

1282-1284: Do not pass entire session_state to the widget; filter to required fields only.

Passing gui.session_state wholesale to render_gooey_builder_inline can inadvertently expose unrelated or sensitive data to the client widget. The commented-out code below (lines 1286-1305) shows the safer, filtered approach.

Apply the diff from the previous review to restore a filtered payload:

 if not self.is_current_user_admin():
     return
-render_gooey_builder_inline(self.slug_versions[-1], gui.session_state)
+builder_state = dict(
+    status=dict(
+        error_msg=gui.session_state.get(StateKeys.error_msg),
+        run_status=gui.session_state.get(StateKeys.run_status),
+        run_time=gui.session_state.get(StateKeys.run_time),
+    ),
+    request=extract_model_fields(
+        model=self.RequestModel, state=gui.session_state
+    ),
+    response=extract_model_fields(
+        model=self.ResponseModel, state=gui.session_state
+    ),
+    metadata=dict(
+        title=self.current_pr.title,
+        description=self.current_pr.notes,
+    ),
+)
+render_gooey_builder_inline(self.slug_versions[-1], builder_state)

This preserves the inline experience while avoiding broad session exposure.

Based on past review comments.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/base.py (1)

1291-1310: Remove commented code.

As noted in the previous review, this commented block should be removed to improve code cleanliness. The inline rendering approach is now established, and keeping the old implementation as a comment adds unnecessary clutter.

Apply this diff to remove the commented code:

-
-        # render_gooey_builder(
-        #     page_slug=self.slug_versions[-1],
-        #     builder_state=dict(
-        #         status=dict(
-        #             error_msg=gui.session_state.get(StateKeys.error_msg),
-        #             run_status=gui.session_state.get(StateKeys.run_status),
-        #             run_time=gui.session_state.get(StateKeys.run_time),
-        #         ),
-        #         request=extract_model_fields(
-        #             model=self.RequestModel, state=gui.session_state
-        #         ),
-        #         response=extract_model_fields(
-        #             model=self.ResponseModel, state=gui.session_state
-        #         ),
-        #         metadata=dict(
-        #             title=self.current_pr.title,
-        #             description=self.current_pr.notes,
-        #         ),
-        #     ),
-        # )

Based on learnings

daras_ai_v2/base.py (1)

419-466: Consider adding a docstring to document sidebar integration.

The render_sidebar method is a new public method but lacks documentation explaining when it's called and its integration with the page wrapper/layout system.

Apply this diff to add documentation:

 def render_sidebar(self):
+    """
+    Render an admin-only sidebar containing the Gooey builder interface.
+    
+    This method is called by the page wrapper to render sidebar content.
+    The sidebar is only displayed on the run tab for admin users and provides
+    an inline builder interface for workflow configuration.
+    """
     if not self.is_current_user_admin():
         return

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

routers/root.py (1)

823-828: Fix className concatenation bug (regression from past review).

Despite the past review comment indicating this was addressed, the className parameter concatenation issue remains:

1. When the sidebar is closed (not is_builder_sidebar_open), the custom className parameter is completely dropped
2. When the sidebar is open, there's no space between "mx-2 w-100" and the custom className

Apply this diff to properly concatenate the className in both cases:

             with gui.div(
                 id="main-content",
-                className="container-xxl "
-                if not is_builder_sidebar_open
-                else "mx-2 w-100" + className,
+                className=(
+                    "container-xxl " + className
+                    if not is_builder_sidebar_open
+                    else "mx-2 w-100 " + className
+                ),
             ):

routers/root.py (1)

733-733: Consider removing redundant assignment.

The line container = page if page else None is redundant since it simply assigns page to container. You can use page directly in the subsequent code.

Apply this diff to simplify:

-    container = page if page else None
     sidebar_ref = use_sidebar("builder-sidebar", request.session, default_open=False)
     sidebar_content, pane_content = sidebar_layout(sidebar_ref)
 
     is_builder_sidebar_open = sidebar_ref.is_open
     with sidebar_content:
-        if container:
-            container.render_sidebar()
+        if page:
+            page.render_sidebar()

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/base.py (1)

1291-1310: Remove the commented legacy render_gooey_builder call

The commented block with the old non‑inline render_gooey_builder(...) call is now redundant and increases noise. Since the inline path is the active implementation, it’s safe to delete this block.

-        # render_gooey_builder(
-        #     page_slug=self.slug_versions[-1],
-        #     builder_state=dict(
-        #         status=dict(
-        #             error_msg=gui.session_state.get(StateKeys.error_msg),
-        #             run_status=gui.session_state.get(StateKeys.run_status),
-        #             run_time=gui.session_state.get(StateKeys.run_time),
-        #         ),
-        #         request=extract_model_fields(
-        #             model=self.RequestModel, state=gui.session_state
-        #         ),
-        #         response=extract_model_fields(
-        #             model=self.ResponseModel, state=gui.session_state
-        #         ),
-        #         metadata=dict(
-        #             title=self.current_pr.title,
-        #             description=self.current_pr.notes,
-        #         ),
-        #     ),
-        # )

daras_ai_v2/gooey_builder.py (1)

7-68: Guard config.onClose against missing #onClose element

The inline builder wires config.onClose to document.getElementById("onClose").click(), which will throw if the element is absent (e.g., layout changes, multiple builders, or timing edge cases). A small null‑check keeps the widget from crashing when the close target isn’t present.

-    config.onClose = function() {
-        document.getElementById("onClose").click();
-    };
+    config.onClose = function() {
+        const btn = document.getElementById("onClose");
+        if (btn) btn.click();
+    };

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

routers/root.py (2)

721-729: Correct page parameter type annotation (currently only allows None)

page: None = None annotates the parameter as literally None, which hides that callers can/should pass a page instance (e.g., BasePage) and degrades type-checker/IDE help.

Consider something like:

-from contextlib import contextmanager
+from contextlib import contextmanager
+from typing import Optional
+from daras_ai_v2.base import BasePage
@@
 def page_wrapper(
     request: Request,
-    page: None = None,
+    page: Optional[BasePage] = None,

(or your preferred union/forward-reference style if circular imports are a concern).

---

829-835: Fix main-content className concatenation (drops custom classes when sidebar is closed)

Right now:

className="container-xxl "
if not is_builder_sidebar_open
else "mx-2 w-100" + className,

• Ignores the passed className when the sidebar is closed.
• Misses a separating space when the sidebar is open.

A more robust pattern:

-            with gui.div(
-                id="main-content",
-                className="container-xxl "
-                if not is_builder_sidebar_open
-                else "mx-2 w-100" + className,
-            ):
+            base_class = (
+                "container-xxl"
+                if not is_builder_sidebar_open
+                else "mx-2 w-100"
+            )
+            extra_class = f" {className}" if className else ""
+            with gui.div(
+                id="main-content",
+                className=base_class + extra_class,
+            ):

so any caller-provided classes are honored in both sidebar states with correct spacing.

routers/root.py (1)

783-799: Hide Gooey builder mobile button on non-recipe pages

The admin-only mobile builder button is rendered for every page using page_wrapper, even when page is None, so on non-recipe pages tapping it can show a blank sidebar.

To avoid dangling UI, you can additionally gate on recipe pages:

-                    if request.user and request.user.is_admin:
+                    if (
+                        is_recipe_page
+                        and request.user
+                        and request.user.is_admin
+                    ):

so the button only appears where the builder actually renders.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/base.py (2)

1291-1310: Remove legacy commented render_gooey_builder block

The fully commented render_gooey_builder(...) block is now redundant given the inline embed path above and adds noise. It’s also the only place in this file that references render_gooey_builder, so keeping it around can confuse future readers.

Consider deleting the entire commented block:

-        # render_gooey_builder(
-        #     page_slug=self.slug_versions[-1],
-        #     builder_state=dict(
-        #         status=dict(
-        #             error_msg=gui.session_state.get(StateKeys.error_msg),
-        #             run_status=gui.session_state.get(StateKeys.run_status),
-        #             run_time=gui.session_state.get(StateKeys.run_time),
-        #         ),
-        #         request=extract_model_fields(
-        #             model=self.RequestModel, state=gui.session_state
-        #         ),
-        #         response=extract_model_fields(
-        #             model=self.ResponseModel, state=gui.session_state
-        #         ),
-        #         metadata=dict(
-        #             title=self.current_pr.title,
-        #             description=self.current_pr.notes,
-        #         ),
-        #     ),
-        # )

(You can then drop the render_gooey_builder import at Line 48 if it’s unused elsewhere.)

---

419-466: Close handler should also clear mobile sidebar state

In render_sidebar, the "onCloseGooeyBuilder" path (Line 441) only calls sidebar_ref.set_open(False). If the sidebar was opened via is_mobile_open, the mobile flag remains True in the session, so the sidebar can get out of sync or immediately re-open on mobile. The tab != run/preview branch correctly clears both flags; the onClose handler should do the same.

Recommend updating the onClose branch:

-            if gui.session_state.pop("onCloseGooeyBuilder", None):
-                sidebar_ref.set_open(False)
-                raise gui.RerunException()
+            if gui.session_state.pop("onCloseGooeyBuilder", None):
+                sidebar_ref.set_open(False)
+                sidebar_ref.set_mobile_open(False)
+                raise gui.RerunException()

Also double-check any other future close-only branches to ensure both flags are reset.

workspaces/models.py (1)

177-177: Consider adding help_text for admin clarity.

The features field is a generic JSON bag. Adding a help_text would help administrators understand its intended use and expected structure.

-    features = models.JSONField(default=dict)
+    features = models.JSONField(default=dict, help_text="Workspace feature flags and configuration")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

routers/root.py (3)

734-734: Type annotation still needs correction despite being marked as addressed.

The past review comment flagged page: None = None as incorrect because it only allows None as a value. While marked as addressed in earlier commits, the provided code still shows this issue. The parameter should accept either None or a page instance.

Apply the suggested fix from the previous review:

 def page_wrapper(
     request: Request,
-    page: None = None,
+    page: typing.Optional["BasePage"] = None,
     className="",
     search_filters: typing.Optional[SearchFilters] = None,
     show_search_bar: bool = True,
     is_recipe_page: bool = False,
 ):

Note: You may need to add from __future__ import annotations at the top of the file or import BasePage directly if not already present.

---

748-752: Mobile sidebar state not considered (duplicate of previous review).

The logic on line 748 sets is_builder_sidebar_open = sidebar_ref.is_open but ignores sidebar_ref.is_mobile_open. This was flagged in a previous review as causing mobile-open sidebars to remain visually open on non-recipe pages.

Apply the suggested fix from the previous review:

-    is_builder_sidebar_open = sidebar_ref.is_open
-    if not is_recipe_page and (is_builder_sidebar_open):
+    is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open
+    if not is_recipe_page and is_builder_sidebar_open:
         sidebar_ref.set_open(False)
         sidebar_ref.set_mobile_open(False)
         raise gui.RerunException()

---

839-845: className concatenation issue still present (duplicate of previous review).

The past review flagged that when the sidebar is closed, the className parameter is dropped, and when open, spacing before custom classes is missing. While marked as addressed, the provided code still shows this issue.

Apply the suggested fix from the previous review:

             with gui.div(
                 id="main-content",
-                className="container-xxl "
-                if not is_builder_sidebar_open
-                else "mx-2 w-100" + className,
+                className=(
+                    "container-xxl " + className
+                    if not is_builder_sidebar_open
+                    else "mx-2 w-100 " + className
+                ),
             ):

widgets/sidebar.py (1)

296-310: Click-to-open handler issue still present (duplicate of previous review).

The past review flagged that the onClick handler checks event.target.id === "sidebar-click-container" but:

1. The wrapper div has no id attribute
2. event.target refers to the clicked element (which may be a child), not the container

This prevents the handler from firing correctly.

Apply the suggested fix from the previous review:

         gui.div(
+            id="sidebar-click-container",
             className="d-flex w-100 h-100 position-relative sidebar-click-container",
             style={"height": "100dvh"},
             onClick=dedent(
                 """
-                if (event.target.id === "sidebar-click-container") {
+                if (event.currentTarget.id === "sidebar-click-container") {
                     document.getElementById("sidebar-hidden-btn").click();
                 }
                 """
                 if not sidebar_ref.is_open
                 else ""
             ),
         ),

daras_ai_v2/gooey_builder.py (1)

7-126: Consider refactoring to eliminate code duplication.

The render_gooey_builder_inline and render_gooey_builder functions share approximately 90% identical code. Only the mode, showRunLink, div style, and onClose handler differ between them.

🔎 Consider this refactoring approach:

Extract common logic into a helper function and parameterize the differences:

def _render_gooey_builder_common(
    page_slug: str, 
    builder_state: dict, 
    mode: str = "default",
    show_run_link: bool = False,
    div_style: str = "",
    on_close_handler: str = ""
):
    """Common logic for rendering Gooey builder."""
    if not settings.GOOEY_BUILDER_INTEGRATION_ID:
        return

    bi = BotIntegration.objects.get(id=settings.GOOEY_BUILDER_INTEGRATION_ID)
    config = bi.get_web_widget_config(
        hostname="gooey.ai", target="#gooey-builder-embed"
    )

    config["mode"] = mode
    if show_run_link:
        config["showRunLink"] = True
    
    branding = config.setdefault("branding", {})
    branding["showPoweredByGooey"] = False

    config.setdefault("payload", {}).setdefault("variables", {})
    variables = dict(
        update_gui_state_params=dict(state=builder_state, page_slug=page_slug),
    )

    gui.html(
        f"""
<div id="gooey-builder-embed" {div_style}></div>
<script id="gooey-builder-embed-script" src="{settings.WEB_WIDGET_LIB}"></script>
        """
    )
    
    mount_call = "GooeyEmbed.mount(config);" if not on_close_handler else f"""
    config.onClose = function() {{
        document.getElementById("onClose").click();
    }};
    GooeyEmbed.mount(config);
    """
    
    gui.js(
        f"""
async function onload() {{
    await window.waitUntilHydrated;
    if (typeof GooeyEmbed === "undefined" ||
        document.getElementById("gooey-builder-embed")?.children.length)
        return;
    
    GooeyEmbed.setGooeyBuilderVariables = (value) => {{
        config.payload.variables = value;
    }};
    GooeyEmbed.setGooeyBuilderVariables(variables);
    
    {mount_call}
}}

const script = document.getElementById("gooey-builder-embed-script");
if (script) script.onload = onload;
onload();
window.addEventListener("hydrated", onload);

if (typeof GooeyEmbed !== "undefined" && GooeyEmbed.setGooeyBuilderVariables) {{
    GooeyEmbed.setGooeyBuilderVariables(variables);
}}
        """,
        config=config,
        variables=variables,
    )

def render_gooey_builder_inline(page_slug: str, builder_state: dict):
    _render_gooey_builder_common(
        page_slug, 
        builder_state, 
        mode="inline",
        show_run_link=True,
        div_style='style="height: 100%"',
        on_close_handler="onClose"
    )

def render_gooey_builder(page_slug: str, builder_state: dict):
    _render_gooey_builder_common(page_slug, builder_state)

routers/root.py (1)

744-744: Unused variable assignment.

The variable container is assigned on line 744 but is never used in the function. It appears this was meant to be used somewhere but was replaced by direct access to page.

🔎 Apply this diff to remove the unused variable:

-    container = page if page else None
     sidebar_ref = use_sidebar("builder-sidebar", request.session, default_open=False)

widgets/sidebar.py (3)

24-26: Remove or document commented code.

Line 26 contains commented code # self.set_open(value). This suggests uncertainty about whether setting mobile open should also set the regular open state. Either remove this if it's not needed, or add a comment explaining why this relationship is intentionally decoupled.

---

128-128: Remove unused loop variable.

The loop variable i is declared but never used in the loop body. This is flagged by static analysis.

🔎 Apply this diff:

-    for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS):
+    for url, label, icon in settings.SIDEBAR_LINKS:

Or if you anticipate using the index in the future:

-    for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS):
+    for _i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS):

Based on learnings, static analysis hints should be addressed when valid.

---

44-46: Use pop() instead of checking existence then deleting.

Static analysis correctly identifies that the pattern of checking if a key exists and then deleting it can be simplified using pop().

🔎 Apply this diff:

-        mobile_key = key + ":mobile"
-        if mobile_key in session:
-            del session[mobile_key]
+        session.pop(key + ":mobile", None)

This is more concise and thread-safe.

Based on learnings, static analysis hints should be addressed when valid.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

widgets/sidebar.py (1)

34-47: Time-based fresh-load detection remains fragile.

This logic uses a 0.5-second threshold to detect fresh page loads, which can be unreliable under varying network conditions or fast navigations.

routers/root.py (2)

751-755: Include mobile-open state in sidebar open check.

is_builder_sidebar_open only checks sidebar_ref.is_open, ignoring sidebar_ref.is_mobile_open. This can leave a visually open but empty sidebar on mobile when navigating to non-recipe pages.

🔎 Apply this diff:

-    is_builder_sidebar_open = sidebar_ref.is_open
-    if not is_recipe_page and is_builder_sidebar_open:
+    is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open
+    if not is_recipe_page and is_builder_sidebar_open:
         sidebar_ref.set_open(False)
         sidebar_ref.set_mobile_open(False)
         raise gui.RerunException()

---

842-847: Fix className concatenation (parameter ignored when sidebar is closed).

When the sidebar is closed, the className parameter is dropped. When open, a space is missing before the custom classes.

🔎 Apply this diff:

             with gui.div(
                 id="main-content",
-                className="container-xxl "
-                if not is_builder_sidebar_open
-                else "mx-2 w-100" + className,
+                className=(
+                    "container-xxl " + className
+                    if not is_builder_sidebar_open
+                    else "mx-2 w-100 " + className
+                ),
             ):

daras_ai_v2/base.py (1)

1257-1259: Close both desktop and mobile sidebar states on builder close.

When the builder's onClose fires, only set_open(False) is called. If the sidebar was opened via mobile, is_mobile_open remains True, causing the sidebar to stay open or get out of sync.

🔎 Apply this diff:

             if gui.session_state.pop("onCloseGooeyBuilder", None):
                 sidebar_ref.set_open(False)
+                sidebar_ref.set_mobile_open(False)
                 raise gui.RerunException()

widgets/sidebar.py (1)

42-44: Use pop() instead of if key in dict followed by del.

As suggested by static analysis, this can be simplified.

🔎 Apply this diff:

-        mobile_key = key + ":mobile"
-        if mobile_key in session:
-            del session[mobile_key]
+        session.pop(key + ":mobile", None)

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

daras_ai_v2/gooey_builder.py (1)

65-68: Fix Ruff formatting issue (already flagged).

The Ruff formatting issue at the end of this function has already been flagged in previous reviews. Please run ruff format . to fix.

widgets/sidebar.py (2)

34-47: Time-based page load detection is fragile (already flagged).

The time-based hack using a 0.5-second threshold to detect fresh page loads has already been flagged as a major issue in previous reviews. This approach is unreliable across different network conditions and navigation patterns.

---

56-58: Fix Ruff formatting issue (already flagged).

The Ruff formatting issue at line 57 has already been flagged in previous reviews and pipeline failures. Please run poetry run ruff format . to fix.

widgets/sidebar.py (1)

43-44: Consider using pop() for more idiomatic dict key removal.

The pattern if key in dict followed by del dict[key] can be replaced with session.pop(mobile_key, None) for more concise and idiomatic code.

🔎 Apply this diff:

-        if mobile_key in session:
-            del session[mobile_key]
+        session.pop(mobile_key, None)

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

widgets/sidebar.py (4)

23-23: Remove or clarify the commented code.

The commented line # self.set_open(value) suggests uncertainty about whether mobile state should sync desktop state. Either uncomment with a clear reason, or remove if no longer needed.

---

43-44: Apply Ruff suggestion to simplify dictionary cleanup.

Use .pop(mobile_key, None) instead of checking key existence followed by deletion.

🔎 Proposed fix

-        if mobile_key in session:
-            del session[mobile_key]
+        session.pop(mobile_key, None)

Based on static analysis hints.

---

50-55: Simplify redundant default_open evaluation.

Line 50 sets session[key] to the resolved boolean value, then lines 54-55 re-evaluate the same logic. Store the result once and reuse it.

🔎 Proposed refactor

     # set the default open state in session here
-    session[key] = bool(session.get(key, default_open))
+    is_open_value = bool(session.get(key, default_open))
+    session[key] = is_open_value
     ref = SidebarRef(
         key=key,
         session=session,
-        is_open=bool(session.get(key, default_open)),
+        is_open=is_open_value,
         is_mobile_open=bool(session.get(key + ":mobile", False)),
     )

---

69-69: Fix typo in variable name.

sidebar_funtion_classes should be sidebar_function_classes (missing 'c' in "function").

🔎 Proposed fix

-    sidebar_funtion_classes = (
+    sidebar_function_classes = (
         "gooey-sidebar-open"
         if sidebar_ref.is_open or sidebar_ref.is_mobile_open
         else "gooey-sidebar-closed"
     )

And update the reference on line 161:

         sidebar_content_placeholder = gui.div(
-            className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}",
+            className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}",
         )

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

widgets/sidebar.py (1)

35-47: Time-based page load detection is fragile and unreliable.

As noted in previous reviews, the time-based approach (checking if 0.5 seconds have elapsed) to determine fresh page loads has several issues:

• Fast navigations may not clear mobile state, causing stale state to persist
• The threshold is arbitrary and may not work reliably across different network conditions
• Multiple rapid requests could interfere with each other

Consider using a more deterministic approach such as a navigation counter, request ID, or an explicit session flag that the caller can reset when needed.

routers/root.py (1)

755-762: Sidebar open state check should consider both desktop and mobile states.

Line 758 sets is_builder_sidebar_open = sidebar_ref.is_open, which only checks the desktop open state. If a user opens the builder via the mobile button (setting is_mobile_open to True), the sidebar will be visually open but is_builder_sidebar_open will be False, causing:

1. The auto-close logic on lines 759-762 won't trigger for mobile-open sidebars on non-recipe pages
2. Header and main content sizing won't react correctly to mobile-open sidebars

The calculation should consider both flags: is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open

This issue was previously flagged and marked as addressed in commits bb057ce to 4e23759, but the current code still shows the incomplete check.

widgets/sidebar.py (1)

43-44: Use .pop() to simplify dict key removal.

The pattern of checking for key existence followed by del can be replaced with a single .pop() call.

🔎 Proposed simplification

-        if mobile_key in session:
-            del session[mobile_key]
+        session.pop(mobile_key, None)

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

routers/root.py (2)

845-850: className concatenation still broken despite past review feedback.

The className parameter passed to page_wrapper is either dropped entirely (when sidebar is closed) or concatenated without proper spacing (when sidebar is open). This was flagged in a previous review (marked as "✅ Addressed in commits 81218fa to df49769") but the fix is not present in the current code.

Current behavior:

• Sidebar closed: "container-xxl " → custom className is lost
• Sidebar open: "mx-2 w-100" + className → missing space separator

Apply the previously suggested fix

-            with gui.div(
-                id="main-content",
-                className="container-xxl "
-                if not is_builder_sidebar_open
-                else "mx-2 w-100" + className,
-            ):
+            with gui.div(
+                id="main-content",
+                className=(
+                    "container-xxl " + className
+                    if not is_builder_sidebar_open
+                    else "mx-2 w-100 " + className
+                ),
+            ):

---

754-762: Mobile sidebar state is still ignored despite past review feedback.

Line 758 only checks sidebar_ref.is_open but ignores sidebar_ref.is_mobile_open. This was flagged in a previous review (marked as "✅ Addressed in commits bb057ce to 4e23759") but the fix is not present in the current code.

When the sidebar is opened via the mobile button, is_mobile_open is True but is_open remains False. This causes:

• The auto-close logic on line 759 to not trigger on non-recipe pages
• Layout calculations in lines 776-779 and 845-850 to use the wrong container classes
• Inconsistent rendering between mobile and desktop states

Apply the previously suggested fix

-    is_builder_sidebar_open = sidebar_ref.is_open
-    if not is_recipe_page and (is_builder_sidebar_open):
+    is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open
+    if not is_recipe_page and is_builder_sidebar_open:
         sidebar_ref.set_open(False)
         sidebar_ref.set_mobile_open(False)
         raise gui.RerunException()

daras_ai_v2/base.py (1)

1232-1241: Consider adding explicit default_open parameter for consistency.

Line 1233 calls use_sidebar without a default_open parameter, while routers/root.py line 755 explicitly passes default_open=False. Since both use the same session key "builder-sidebar", they reference the same sidebar state, so this works correctly. However, adding the explicit parameter improves code clarity and consistency.

Optional consistency improvement

-        sidebar_ref = use_sidebar("builder-sidebar", self.request.session)
+        sidebar_ref = use_sidebar("builder-sidebar", self.request.session, default_open=False)

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

routers/root.py (1)

754-754: Simplify redundant assignment.

The ternary operator is unnecessary since page is already Optional[BasePage] and defaults to None.

🔎 Proposed simplification

-    container = page if page else None
+    container = page

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

routers/root.py (1)

10-11: Top-level import may increase server import time.

The PR description specifically mentions checking that "changes have not increased server import time" and suggests importing heavy libraries inside functions. Consider whether importing render_gooey_builder_launcher at module level is necessary, or if it could be deferred to where it's used (inside page_wrapper).

widgets/sidebar.py (2)

30-58: Time-based fresh page detection is fragile.

The "HUGE HACK" comment is apt—relying on elapsed time (0.5s) to detect fresh page loads can fail under slow network conditions, server latency, or aggressive polling. Consider if there's a more reliable mechanism (e.g., a dedicated session flag cleared on navigation, or checking a request header).

---

21-23: Commented-out code suggests incomplete logic.

The commented line # self.set_open(value) in set_mobile_open might indicate that mobile and desktop open states should be synchronized. Clarify the intended behavior or remove the dead comment.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

widgets/sidebar.py (2)

30-57: Minor redundancy in session state initialization.

Line 49 sets session[key], then line 53 reads it back with session.get(key, default_open). Since the key is already set, the second call will always return session[key], making the default parameter unused.

Suggested simplification

     # set the default open state in session here
-    session[key] = bool(session.get(key, default_open))
+    is_open = bool(session.get(key, default_open))
+    session[key] = is_open
     ref = SidebarRef(
         key=key,
         session=session,
-        is_open=bool(session.get(key, default_open)),
+        is_open=is_open,
         is_mobile_open=bool(session.get(key + ":mobile", False)),
     )

---

66-72: Consider renaming sidebar_function_classes for clarity.

The variable name sidebar_function_classes is misleading since it represents open/closed state, not function. A name like sidebar_state_classes would be clearer.

routers/root.py (1)

770-781: Consider extracting header rendering logic for readability.

The deeply nested context managers (4+ levels) make the header structure difficult to follow. Consider extracting the header content into a separate helper function.

daras_ai_v2/gooey_builder.py (1)

13-20: Return type may be inconsistent.

Line 20 returns current_workspace and current_workspace.enable_bot_builder, which could return None, False, or the enable_bot_builder value. For a function declared to return bool, consider an explicit boolean cast.

Suggested fix

-    return current_workspace and current_workspace.enable_bot_builder
+    return bool(current_workspace and current_workspace.enable_bot_builder)

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.